Skip to content

Revert "[SLP]Check if the copyable element is a sub instruciton with abs in isCommutable" #168148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Revert "[SLP]Check if the copyable element is a sub instruciton with abs in isCommutable" #168148

wants to merge 1 commit into from

Conversation

@Prabhuk
Copy link
Contributor

@Prabhuk Prabhuk commented Nov 14, 2025

This reverts commit ddf5bb0.

@llvmbot
Copy link
Member

llvmbot commented Nov 14, 2025 •
edited
Loading

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-vectorizers

Author: Prabhu Rajasekaran (Prabhuk)

Changes

This reverts commit ddf5bb0.

Full diff: https://github.com/llvm/llvm-project/pull/168148.diff

2 Files Affected:

  • (modified) llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp (+4-7)
  • (removed) llvm/test/Transforms/SLPVectorizer/AArch64/non-inst-abs-sub-copyable-value.ll (-29)
diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index 938eacde7548d..e61eb0fcfe492 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -537,8 +537,7 @@ static bool isSplat(ArrayRef<Value *> VL) {
 /// \param I The instruction to check for commutativity
 /// \param ValWithUses The value whose uses are analyzed for special
 /// patterns
-static bool isCommutative(Instruction *I, Value *ValWithUses,
-                          bool IsCopyable = false) {
+static bool isCommutative(Instruction *I, Value *ValWithUses) {
   if (auto *Cmp = dyn_cast<CmpInst>(I))
     return Cmp->isCommutative();
   if (auto *BO = dyn_cast<BinaryOperator>(I))
@@ -547,7 +546,7 @@ static bool isCommutative(Instruction *I, Value *ValWithUses,
             !ValWithUses->hasNUsesOrMore(UsesLimit) &&
             all_of(
                 ValWithUses->uses(),
-                [&](const Use &U) {
+                [](const Use &U) {
                   // Commutative, if icmp eq/ne sub, 0
                   CmpPredicate Pred;
                   if (match(U.getUser(),
@@ -556,11 +555,10 @@ static bool isCommutative(Instruction *I, Value *ValWithUses,
                     return true;
                   // Commutative, if abs(sub nsw, true) or abs(sub, false).
                   ConstantInt *Flag;
-                  auto *I = dyn_cast<BinaryOperator>(U.get());
                   return match(U.getUser(),
                                m_Intrinsic<Intrinsic::abs>(
                                    m_Specific(U.get()), m_ConstantInt(Flag))) &&
-                         ((!IsCopyable && I && !I->hasNoSignedWrap()) ||
+                         (!cast<Instruction>(U.get())->hasNoSignedWrap() ||
                           Flag->isOne());
                 })) ||
            (BO->getOpcode() == Instruction::FSub &&
@@ -3166,8 +3164,7 @@ class BoUpSLP {
         bool IsInverseOperation = false;
         if (S.isCopyableElement(VL[Lane])) {
           // The value is a copyable element.
-          IsInverseOperation =
-              !isCommutative(MainOp, VL[Lane], /*IsCopyable=*/true);
+          IsInverseOperation = !isCommutative(MainOp, VL[Lane]);
         } else {
           assert(I && "Expected instruction");
           auto [SelectedOp, Ops] = convertTo(I, S);
diff --git a/llvm/test/Transforms/SLPVectorizer/AArch64/non-inst-abs-sub-copyable-value.ll b/llvm/test/Transforms/SLPVectorizer/AArch64/non-inst-abs-sub-copyable-value.ll
deleted file mode 100644
index d90873f1895e4..0000000000000
--- a/llvm/test/Transforms/SLPVectorizer/AArch64/non-inst-abs-sub-copyable-value.ll
+++ /dev/null
@@ -1,29 +0,0 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
-; RUN: opt -passes=slp-vectorizer -S -slp-threshold=-100 -mtriple=arm64-apple-macosx15.0.0 < %s | FileCheck %s
-
-define i1 @test(i32 %shr.i.i90, i32 %x) {
-; CHECK-LABEL: define i1 @test(
-; CHECK-SAME: i32 [[SHR_I_I90:%.*]], i32 [[X:%.*]]) {
-; CHECK-NEXT:  [[ENTRY:.*:]]
-; CHECK-NEXT:    [[SUB32_I_I:%.*]] = sub i32 [[X]], 2
-; CHECK-NEXT:    [[TMP0:%.*]] = insertelement <2 x i32> poison, i32 [[SHR_I_I90]], i32 1
-; CHECK-NEXT:    [[TMP1:%.*]] = insertelement <2 x i32> [[TMP0]], i32 [[SUB32_I_I]], i32 0
-; CHECK-NEXT:    [[TMP2:%.*]] = call <2 x i32> @llvm.abs.v2i32(<2 x i32> [[TMP1]], i1 true)
-; CHECK-NEXT:    [[TMP3:%.*]] = zext <2 x i32> [[TMP2]] to <2 x i64>
-; CHECK-NEXT:    [[TMP4:%.*]] = icmp ugt <2 x i64> [[TMP3]], <i64 100, i64 300>
-; CHECK-NEXT:    [[TMP5:%.*]] = extractelement <2 x i1> [[TMP4]], i32 0
-; CHECK-NEXT:    ret i1 [[TMP5]]
-;
-entry:
-  %cond.i.i = tail call i32 @llvm.abs.i32(i32 %shr.i.i90, i1 true)
-  %conv.i.i91 = zext i32 %cond.i.i to i64
-  %sub32.i.i = sub i32 %x, 2
-  %cond41.i.i = tail call i32 @llvm.abs.i32(i32 %sub32.i.i, i1 true)
-  %conv42.i.i = zext i32 %cond41.i.i to i64
-  %cmp.not.i.2.i.i = icmp ugt i64 %conv.i.i91, 300
-  %cmp.not.i.3.i.i = icmp ugt i64 %conv42.i.i, 100
-  ret i1 %cmp.not.i.3.i.i
-}
-
-; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
-declare i32 @llvm.abs.i32(i32, i1 immarg) #0

@Prabhuk
Copy link
Contributor Author

Prabhuk commented Nov 14, 2025

@alexey-bataev your commit to main broke our builders in Google. Since I could not find a PR for this change to discuss this I am directly reverting this patch. Please let me know if we can help in anyway to help you reland this but a PR would help.

Exit Code: 1

Command Output (stdout):
--
# RUN: at line 2
/b/s/w/ir/x/w/llvm_build/bin/opt -passes=slp-vectorizer -S -slp-threshold=-100 -mtriple=arm64-apple-macosx15.0.0 < /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Transforms/SLPVectorizer/AArch64/non-inst-abs-sub-copyable-value.ll | /b/s/w/ir/x/w/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Transforms/SLPVectorizer/AArch64/non-inst-abs-sub-copyable-value.ll
# executed command: /b/s/w/ir/x/w/llvm_build/bin/opt -passes=slp-vectorizer -S -slp-threshold=-100 -mtriple=arm64-apple-macosx15.0.0
# executed command: /b/s/w/ir/x/w/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Transforms/SLPVectorizer/AArch64/non-inst-abs-sub-copyable-value.ll
# .---command stderr------------
# | /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Transforms/SLPVectorizer/AArch64/non-inst-abs-sub-copyable-value.ll:8:15: error: CHECK-NEXT: expected string not found in input
# | ; CHECK-NEXT: [[SUB32_I_I:%.*]] = sub i32 [[X]], 2
# |               ^
# | <stdin>:7:7: note: scanning from here
# | entry:
# |       ^
# | <stdin>:7:7: note: with "X" equal to "%x"
# | entry:
# |       ^
# | <stdin>:10:2: note: possible intended match here
# |  %2 = sub <2 x i32> %1, <i32 2, i32 0>
# |  ^
# | 
# | Input file: <stdin>
# | Check file: /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Transforms/SLPVectorizer/AArch64/non-inst-abs-sub-copyable-value.ll
# | 
# | -dump-input=help explains the following input dump.
# | 
# | Input was:
# | <<<<<<
# |           1: ; ModuleID = '<stdin>' 
# |           2: source_filename = "<stdin>" 
# |           3: target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-n32:64-S128-Fn32" 
# |           4: target triple = "arm64-apple-macosx15.0.0" 
# |           5:  
# |           6: define i1 @test(i32 %shr.i.i90, i32 %x) { 
# |           7: entry: 
# | next:8'0           X error: no match found
# | next:8'1             with "X" equal to "%x"
# |           8:  %0 = insertelement <2 x i32> poison, i32 %x, i32 0 
# | next:8'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |           9:  %1 = insertelement <2 x i32> %0, i32 %shr.i.i90, i32 1 
# | next:8'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |          10:  %2 = sub <2 x i32> %1, <i32 2, i32 0> 
# | next:8'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | next:8'2      ?                                      possible intended match
# |          11:  %3 = call <2 x i32> @llvm.abs.v2i32(<2 x i32> %2, i1 true) 
# | next:8'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |          12:  %4 = zext <2 x i32> %3 to <2 x i64> 
# | next:8'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |          13:  %5 = icmp ugt <2 x i64> %4, <i64 100, i64 300> 
# | next:8'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |          14:  %6 = extractelement <2 x i1> %5, i32 0 
# | next:8'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |          15:  ret i1 %6 
# | next:8'0     ~~~~~~~~~~~
# |           .
# |           .
# |           .
# | >>>>>>
# `-----------------------------
# error: command failed with exit status: 1

@alexey-bataev
Copy link
Member

alexey-bataev commented Nov 14, 2025

Already reverted

@Prabhuk
Copy link
Contributor Author

Prabhuk commented Nov 14, 2025

@alexey-bataev - thank you!

@Prabhuk Prabhuk closed this Nov 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-bataev alexey-bataev Awaiting requested review from alexey-bataev

Assignees

No one assigned

Labels

llvm:transforms vectorizers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Prabhuk @llvmbot @alexey-bataev